Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Suggesting best routes #40

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Krishi-02
Copy link
Contributor

Closes #17
image

@netlify
Copy link

netlify bot commented Jul 29, 2022

Deploy Preview for prep-22-jul-prep-2-project ready!

Name Link
🔨 Latest commit c06e155
🔍 Latest deploy log https://app.netlify.com/sites/prep-22-jul-prep-2-project/deploys/62e3dfd54d76f200092605de
😎 Deploy Preview https://deploy-preview-40--prep-22-jul-prep-2-project.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sumana2001 sumana2001 requested a review from joshi-kaushal July 29, 2022 15:57
import "leaflet-routing-machine";

function createRoutineMachineLayer({geocodes}) {
console.log(geocodes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove console.log if you are done with testing

@@ -0,0 +1,23 @@
import leaflet from "leaflet";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire component could be shifted to /Components since it does not return a webpage/JSX.

fetch(`https://geocode.search.hereapi.com/v1/geocode?q=${toCity}&apiKey=${process.env.REACT_APP_HEREAPI}`)
.then((res) => res.json())
.then((result) => {
//console.log(result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comment, also remove it from line 28

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two console logs on lines 15 and 39 of /Search.js. I know you have not made those changes. But if possible please remove those as well.

@@ -1,18 +1,91 @@
import React from "react";
import React, {useState} from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can import useEffect here so you don't have to write React.useEffect() everytime.

Also, React is already imported at the global scope for all /src files for React v16+ (I could be technically wrong here, what I mean is you don' need to import React explicitly in /src/ files.



window.navigator.geolocation.getCurrentPosition((position) => {
fetch(`https://api.openweathermap.org/data/2.5/weather?lat=${position.coords.latitude}&lon=${position.coords.longitude}&appid=${process.env.REACT_APP_APIKEY}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar code exists in /screen/home.js. You can receive coordinates and city as props and save this API call.

CC: @Krishi-02

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: you are using lon to denote longitude whereas the rest of the app is using lng.

If you can replace your lon with lng, that would be great. Otherwise, don't forget to change the name of that key.

@@ -21,6 +21,7 @@
npm-debug.log*
yarn-debug.log*
yarn-error.log*
yarn.lock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sumana2001, should we keep this file in gitignore or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to keep

@joshi-kaushal
Copy link
Contributor

Clicking on the find route button is causing an error. This is clearly because you used something related to react-leaflet outside <MapContainer />. However, I can't find where exactly you have used it.

One thing you can try is implementing routing.js as a React component and returning something (worst case: null) from it. And use it inside <MapContainer />

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggesting best routes
4 participants